Added non-blocking radio receive #485
Conversation
|
Thanks much for the contribution! Sorry about the CI failure, which is new: PR #486 should fix it shortly, after which you'll need to rebase. Should this be adapted to support the |
Yes, that's a good idea :) I'll get on to it. |
eivindbergem
force-pushed
the
non-blocking-recv
branch
from
d3d4313
to
19adf7a
Compare
|
If let recv = radio.recv_non_blocking(&mut packet);
recv.is_done(); // -> WouldBlock
recv.is_done(); // -> Ok(crc)
while recv.is_done() == Err(nb::Error::WouldBlock) {} // Would never leave loopThis is not great. I could add some internal state and another error to indicate that we already have received: enum RecvError {
CrcFailure(u16),
AlreadyReceived,
}Or, maybe subsequent calls to |
|
Is this with |
|
This applies to both, with and without I'm not sure if it matters that much anyway, I was just thinking out loud. Futures suffer from the same issue, and the documentation makes it clear that futures that have returned |
eivindbergem
force-pushed
the
non-blocking-recv
branch
3 times, most recently
from
8798adc
to
8a8096a
Compare
|
I refactored |
nrf-hal-common/src/ieee802154.rs
Outdated
| /// Receives one radio packet and copies its contents into the given `packet` buffer | ||
| /// | ||
| /// This method is non-blocking | ||
| pub fn recv_non_blocking(&mut self, packet: &mut Packet) -> Recv<'_, 'c> { | ||
| // Start the read | ||
| // NOTE(unsafe) We block until reception completes or errors |
There was a problem hiding this comment.
This is no longer true. start_recv doesn't have its safety conditions documented, but I suspect it requires the packet reference passed in to live at least until the receive completes. You don't enforce that here, so recv_non_blocking is unsound. You either need to make it unsafe (and document that the caller must ensure the packet lives at least until the receive operation completes), or add some safe abstraction to ensure that it does, such as having the Radio handle allocation of the packet.
There was a problem hiding this comment.
Good catch! I've added a mutable reference to the packet in Recv to enforce this.
There was a problem hiding this comment.
Unfortunately that's still not sound. The caller can call forget on the returned Recv and then drop the Packet that they passed a reference to, while the hardware still tries to write to it.
There was a problem hiding this comment.
If we require a pinned pointer for Packet, we get a drop guarantee.
There was a problem hiding this comment.
No, that won't help as we can still forget Recv and then drop the Packet. It's the Recv that needs to be dropped, not Packet. I guess unsafe is the best option then.
There was a problem hiding this comment.
Tried something new. recv_non_blocking takes a closure that takes &Recv as argument.
There was a problem hiding this comment.
Interesting approach, it's not a pattern I've seen much before for something like this but I think it's sound.
There was a problem hiding this comment.
If it's a soundness issue we might want to get a second review before we merge. I can try, but I'm bad at that stuff: might want to tag somebody?
eivindbergem
force-pushed
the
non-blocking-recv
branch
3 times, most recently
from
9ac3659
to
c1f33f2
Compare
eivindbergem
force-pushed
the
non-blocking-recv
branch
from
c1f33f2
to
a38dcc6
Compare
I've added a simple non-blocking receive. The method returns a struct that takes care of checking status and calling
cancel_recv(). Now it callscancel_recv()for every drop. I can add some state to avoid this if needed.I didn't put much thought into the names, so please feel free to bikeshed.